Add Loris Chiocca's IBM Music Feature Card (IMFC) patch #2367
Conversation
Loris Chiocca: - Authored the IBM Music Feature card (IMFC) emulator, as follows: - Reverse-engineered the IMF's Z80 ROM and tested against hardware. - Used IDA Pro (licensed) to help structure and anonotate the assembly. - Ported the assembly to C++ (without IDA Pro's assembly-to-C module). - Integrated the results into a working DOSBox 0.74.3 patch. - Used the GPL v2+ Virtual FB-01 project for FB-01 emulation (below). Daisuke Nagano: - Authored the Virtual FB-01 project (sources inline here). - Used the YM-2151 emulator from MAME's GPL v2+ sources (below). Jarek Burczynski: - Authored the YM-2151 emulator in MAME, which is used in the Virtual FB-01 project and dervices sources here. --- Given both the Virtual FB-01 and YM-2151 sources are included in the integrated IMFC emulator, their authors are attributed as co-authors on this commit: Co-authored-by: Daisuke Nagano <<breeze.nagano@nifty.ne.jp> Co-authored-by: Jarek Burczynski <s0246@priv4.onet.pl> Signed-off-by: kcgen <kcgen@users.noreply.github.com>
Temporary remove initialization from dosbox.cpp; this will be re-added in a future commit when it's passing DOSBox Staging CI gates.
In the previous implementation, SDL1's forceful thread termination call was used to break the main thread out of its endless while(true) loop. However, SDL2 doesn't have a forceful thread halt routine. This commit introduces an atomic bool that both threads can watch and exit when its state changes.
There were only a couple cases of this. Isolated in a separate commit in the rare case this changes functionality. For example: "#define CUBE(I) (I * I * I)" The invocation: int a = CUBE(2 + 1); Expands to: int a = (2 + 1 * 2 + 1 * 2 + 1); (a == 7, instead of 9) Another reason why constexpr should be preferred :-)
This replaces the double-underscore with
single-underscores which side-steps this issue:
Wording from Clang:
The C and C++ standards both reserve the following names
for such use:
identifiers that begin with an underscore followed by
an uppercase letter;
identifiers in the global namespace that begin with
an underscore.
The C standard additionally reserves names beginning with
a double underscore, while the C++ standard strengthens
this to reserve names with a double underscore occurring
anywhere.
Clang's wording: This check flags user-defined constructor definitions that do not initialize all fields that would be left in an undefined state by default construction, e.g. builtins, pointers and record types without user-provided default constructors containing at least one such type. If these fields aren’t initialized, the constructor will leave some of the memory in an undefined state. For C++11 it suggests fixes to add in-class field initializers. For older versions it inserts the field initializers into the constructor initializer list. It will also initialize any direct base classes that need to be zeroed in the constructor initializer list.
Clang's wording:
Checks that constructors callable with a single argument
and conversion operators are marked explicit to avoid the
risk of unintentional implicit conversions.
Consider this example:
struct S {
int x;
operator bool() const { return true; }
};
bool f() {
S a{1};
S b{2};
return a == b;
}
The function will return true, since the objects are
implicitly converted to bool before comparison, which is
unlikely to be the intent.
The check will suggest inserting explicit before the
constructor or conversion operator declaration.
(these can be later dropped or moved to [[maybe_unused]] if they're conditionally)
This is just an initial cleanup step to ensure they don't need to be macros, and can be moved to constexpr (or other forms) later.
I'm not sure why references weren't passed in, but if there's a reason, this approach of moving the copied string argument is still safe and roughly accomplishes the same thing as using a reference.
Yes.. this exposes how unreadable bool literals; so be it. They can be explicitly named or fixed in subsequent commits.
(Just uses brace initialization, can be cleaned up later)
These functions don't operate on any member variables and therefore are just plain functions that happen to be co-located inside a struct or class (which is fine). The static helps readers understand that these are stateless.
@FeralChild64 Let me give @kcgen a hand. This is what ChatGPT's summary of all the commit names from the pull request above: The pull request, titled "Add Loris Chiocca's IBM Music Feature patch #2367", integrates the IBM Music Feature (IMF) card support into the DOSBox Staging. It includes multiple commits that refactor and optimize the code, addressing various aspects such as integer types, explicit namespace usage, macro handling, initialization, const correctness, header names, loops, constructors, and more. Additionally, it incorporates changes to make the code conform to C++ standards and DOSBox Staging conventions. The commits also cover improvements in the code's functionality, such as configuring initial settings from byte streams, standardizing mixer rendering, and adding settings for IMFC IRQ, port, and filter. |
|
@ThomasEricB OK, now I understand. I didn’t realize „IBM Music Feature” is a sound card name, and Google search did not find anything useful :) |
|
The PR includes the clean-room reverse engineered assembly structures and functions (over 20k LOC, in the contrib/ area). The assembly is annotated with names that map over to the C++ code. I think we can try a couple small style adjustments - but my main worry is that we break this relationship for anyone trying to work with the assembly in some systematic way. Another option is that we put a note at the top about how the style is "per the original code". |
|
LGR Space Quest 3: https://www.youtube.com/watch?v=lyIBfjsTZcQ&t=144s PR's Space Quest 3 (w/ reverb and chorus enabled): sciv_002.mp4 |
I've watched the video fully now. @kcgen have you seen this: https://github.com/Scalibq/FB-01-Emulator . Perhaps we want to emulate FB-01 Yamaha as well, since it has more support than the IBM Music Feature's handful of Sierra titles? EDIT: Found a VOGON's thread about it: https://www.vogons.org/viewtopic.php?t=9555&start=40 |
That's a great thread 👍 and to add to it, Scali has a very thorough article about his adventures in IMFC and FB-01 land, emulating the latter and included a video showing all the pieces in play: https://scalibq.wordpress.com/2019/03/26/the-ibm-music-feature-card-and-yamaha-fb-01/ |
kcgen
changed the title
|
I randomly
@johnnovak - although I would like to have consistent naming patterns and style, I'm weighing this against the risks (or damage done) to anyone wanting to work/learn/verify the assembly with the code, and benefits of connecting the assembly in IDA Pro (or any other systematic code processing approaches). I think all fixes not involving renaming (whitespace, |
Right, wasn't aware of that. In that case best to leave the names alone, 100%. |
|
This is a very nice addition, and Silpheed sounds really good on the card! I'm wondering if there's much point in continuing with my review because I can only comment on superficial stuff, and I agree we should keep the symbol names in sync with the asm code. So I'm fine for you to merge as it is. |
- Drops the module class - Moves the IO handling inside class - Adjusts the constructor to take in the audio channel, IO port, and IRQ number (similar to other DOSBox Staging audio devices).
100%!
I've addressed almost all of them that don't hit the assembly, but agree there's not a lot more to do regarding naming changes. Updated the branch; now switched over to |
|
Just one remark from my side. |
|
For those interested in matching the IMFC hardware recordings shared by Pierre on YouTube, you can try setting For example, here's Pierre's recording taken from the hardware: pierra-imfc-ymfb01-colonels-bequest-intro.mp4Here's DOSBox Staging 0.81 (alpha) with cap-2.1vol-44100-hz-filter-lpf-2-3500Hz-chorus-80.mp4And here's a visual comparison of them:
|
This PR imports Loris Chiocca's IBM Music Feature Card emulator - a rare and expensive audio card (even for the 80s!) made by IBM that was primarily used by the early Sierra EGA (SCI0) games.
Thank you, @lchiocca, for the great project, sharing your background story, and helping document the authorship history!
The card runs its own firmware on a Z80 processor making calls to a Yamaha FM chip to generate instrument banks and custom sounds, but can also have FM banks uploaded.
LGR just released a video about the card: https://www.youtube.com/watch?v=lyIBfjsTZcQ
Sierra's musicians did a decent job with it (look for the
IMF.DRVfile in their games); NerdlyPleasures has two thorough articles about it: http://nerdlypleasures.blogspot.com/2015/02/the-ibm-music-feature-card-overpriced.html and http://nerdlypleasures.blogspot.com/2017/07/new-discoveries-about-ibm-music-feature.htmlHere's the configuration file block:
Like other audio cards, you can turn it on manually with the command
imfc on(and off withimfc off). It'll show up in the mixer:Here's a quick example using Silpheed, whose composers did a great job with it (don't mind my distracted piloting 😅 , I was enjoying the music!):
sierra_001.mp4
All the add-on commits primarily address CI gates and getting it tied into Staging's slightly changed mixer, IO binds, and more direct init approach.