mist-devel / mist-firmware

Firmware source code for the MIST board
80 stars 39 forks source link

Discussion: variable config string #42

Closed jotego closed 4 years ago

jotego commented 4 years ago

Let me bring up another topic for discussion: the config string. I have been using it to set the DIP settings of arcade games so far. As it has always been one game per RBF file, it worked out well.

However for CPS1 titles I face the issue of having 30+ games running on the same RBF so I cannot have a fixed DIP setting definition in the config string.

In MiSTer they have solved this issue by using the keyword DIP; in the config string. This tells the firmware to read DIP settings from the MRA file which triggered the core. I understand we are not implementing MRA file reading on MiST and indeed the stand alone tool to generate binary files from an MRA file is working quite well.

Given the circumstances, how would you suggest to tackle this problem? Can I vary the config string on the fly after a new ROM load?

gyurco commented 4 years ago

In theory, it is possible, since the firmware doesn't store the whole string, but constantly requests it via SPI. But what race conditions will happen if you change it on the fly is unknown. Are you planning to release one RBF for several arcades? Because if there will be one RBF/game, then a compile-time setting would be more feasible.

gyurco commented 4 years ago

I see another problem with changing config string: if you change the core name on the fly, the settings won't loaded automatically from the .cfg. If you don't change it, then different games will overwrite the settings of another game. Also, just loading a ROM won't switch to another game, unless you put some auto-detection method in the core. At the end, the best approach so far: provide compile-time changing config string, and build different RBF for different games (that will have the advantage that it'll load the correct ROM file). If one game supports more variants, but with different DIP settings - that's unlucky.

jotego commented 4 years ago

Capcom system 1 supports about 32 games. And I think I can make it work with just one RBF file. A different approach to changing the config string is to have the firmware read a different status word for each game. The name of the 32 bit status file could be derived from the ROM name. Yet another approach would be to have MiST read the MRA just for selecting the RBF file, the name of the pre-generated ROM file and the config string information. That is simpler than processing the ROM generation section.

gyurco commented 4 years ago

Ok, then the firmware will aware of a different core name (not what the FPGA supplies in the config string), and some config string setting. But the FPGA won't know about the variant.

What about an even simpler method (at least in the firmware): just supply a file with the core name and a variant number, e.g. gng.arc:

cps1.rbf=1

finalfight.arc:

cps1.rbf=2

The core will receive this number before the config string is requested, then it can choose the appropriate one.

That's surely has a negative impact on the core size, but the config string can be implemented in BRAM, if it will consume too much logic.

Upd: maybe it's not even affecting the core size too much. I think there are only a few names for DIP settings, and just need to disable/enable (send to the firmware) them selectively.

harbaum commented 4 years ago

Imho it's even simpler. You are right that the way to distinguish variant/ROM set is the cores name. And this is used to load the rom set. And we have the new MRA tool with the DIP setting already present in the MRA file. So let the MRA converter just append the dip setting as a few extra bytes to the rom. The core can then load the rom with the dip settings. You could even add default dip settings to the core which stay sctive until the rom upload with the extra bits overwrites them.

gyurco commented 4 years ago

It would be interesting to put the config string into the ROM. However I see a little race condition here: the ROM name is determined by the config string itself. But maybe it doesn't matter, as only the first line used for the core name detection. But if the external description file (let's call it ARC file, like Arcade) works with supplying a variant ID, it's up the core developer how to implement the differences between variants.

harbaum commented 4 years ago

In see the config string as the very fundamental thing sent by the core to the firmware to get things going. The ROM is a collection of fixed data sent from the firmware to the core (opposite direction). I wouldn't mix these as - like you said - you'd get a circular dependency.

gyurco commented 4 years ago

Here's the first working version of the firmware mod (the parser is very simple, no comments, spaces, etc. allowed): https://github.com/gyurco/mist-firmware/commit/803343334818b7577a405cefee4436a189cb77ad And the first core, which uses variants is Scramble hardware. The patch with the user_io and top-level mods: https://github.com/gyurco/Mist_FPGA/commit/05170088f7ce9eae17c089f85e3b65d761e6e76d

@jotego please check if it's adequate for your needs.

I know DIP switches are not included, and string manipulations in Verilog are not the most interesting things to do, but that's what can be done without significant increasing the memory consumption inside the firmware.

jotego commented 4 years ago

Hi, I have just read everything, checked the files and spend a few minutes understanding everything. Let me summarize it here:

  1. There is a new file type called .arc that contains an RBF filename plus a number.
  2. The firmware recognizes these files, loads the RBF and sends the number before requesting the string
  3. The core can then generate a binary stream (i.e. a string of bytes) with different data dependent on the variant.

I think that's pretty impressive and it opens up a lot of possibilities.

That actually is rather complex for CPS as the number of unique games is large (30+), DIP configurations are large (3x8 switches) and I guess some game variations may have different DIP settings. So I think the memory requirement can explode very quickly into something that requires a full BRAM and thus an user_io module that reads the string from a memory and not a parameter.

If there is going to be a full blown BRAM memory, I'd rather have the DIP strings stored in the MRA as Harbaum proposed. That makes the hardware handling of the data more straight forward: fill the DIP memory during ROM downloading and use the ARC number to select an offset in the memory.

If we are going to have a new .arc file... why not?

  1. Write DIP settings directly to it, with the same format used in the config string so it can share the same parser Or
  2. Write the ROM file name on it, so it can be sent to the FPGA without the need of the config string. Then the FPGA will get the config string data from the ROM file and will be able to send it back to the MCU
gyurco commented 4 years ago

Modifying user_io to read from BRAM is very easy, and that was already done for SNES where I was out of LCs already. One M9K can store more than 1000 chars, hopefully it's enough. I expect that there would be many common string, like "O6,Service,Off,On;". Or at the end, some hundred bytes of firmware RAM can be spent on storing a config string snippet from the ARC file (even the whole config string, if let's say we allow one sector - 512 bytes - of ARC data). That would be really more developer-friendly than doing string manipulation in HDL.

And just a stupid idea: send the config string from the ARC file to the FPGA with the mod number, then the core can send back to the firmware when it's requested :)

Meanwhile I think about one possible issue with the external config string: testing the core loading with USB-Blaster is not really possible. While one mod number is easy to override for debugging, config string options are not that convenient.

I think for the moment, let's settle with the simple ARC format (rbfname=mod), and an user_io change where you can control the config string from your code, then we can see from real-world usage if it should be extended or it is enough.

gyurco commented 4 years ago

I've added a DIP-switch concept: https://github.com/gyurco/Mist_FPGA/commit/aa5bdd12f3d684e67399871c66f1ca955076aeca Now if STRLEN=0 in user_io, it's totally up to the top-level how to construct the config string. With appropriate synchronous logic, it's even possible to automatically infer it to BRAM.

harbaum commented 4 years ago

I understand what you are doing and how. But i must admit that I think this is not the clean solution.

We basically have two things for each core: Hardware and software. The core implements the hardware and the ROM implements the software. The dip switches themselves are hardware but their configuration is imho part of the software as you'd set them with the ROMs you install.

So the core itself should imho not contain any dip settings. Instead it should just receive those with the ROM. You could then just upload a different rom set with new dip settings and would not have to resynthesize the core to e.g. support the dip settings of a new rom set. There may even be different rom sets of the same game requiring different dip settings.

gyurco commented 4 years ago

I understand. Just embedding config snippets in the ROM file currently cause more trouble than the problem it solves.

gyurco commented 4 years ago

Thinking about more, embedding the config snippet into the .ARC file would be the best way. Let's limit it to 256 bytes, then it's not a big impact. Should be enough for one game, I think. This way also it doesn't have to travel twice, like embedding in the ROM, e.g. SD Card->ARM->FPGA>ARM, but only SD Card->ARM. This makes debugging via USB-Blaster a bit uncomfortable (you won't see that part of the menu), but I think that's not the world's end. Other plus is don't have to modify the mra tool to embed the config. And finally it won't consume any FPGA resources.

harbaum commented 4 years ago

Sounds good to me. That way we can hide settings which aren't useful with a certain ROM set.

But where does the core name reside? Is there a default config string in the core with at least the core name?

gyurco commented 4 years ago

Yes, the same idea as @jotego mentioned in the first post would be used: a placeholder DIP; in the config string from the FPGA will be replaced with the config snippet in the ARC file. Could be a better name than DIP.

gyurco commented 4 years ago

Now the question is just the format of the ARC file: RBFNAME=mod no. $CONFIG SNIPPET....(continues until the file end, maximized in 256 bytes).

Or use ini? [arc] rbf=RBFNAME mod=MODNO confstr=...

The problem is there's a limit in the line length in the ini parser. Could be confstr1=.., confstr2=.., but that's a bit ugly in my eyes.

Or who knows... confstr[0-9]=... e.g.: confstr0=O7,Service,Off,On; confstr1=O8,Lives,3,5;

theflynn49 commented 4 years ago

consider JSON ...

harbaum commented 4 years ago

Use JSON because we need to save ram and flash space?

gyurco commented 4 years ago

Then we could use the original XML, no need for converters and such :)

theflynn49 commented 4 years ago

gyurco has a point, but by experience, a limited version of Json has its advantages. But it was just my 2¢

gyurco commented 4 years ago

Tell that the Mr. guys who created that XML format at the first place. Also I don't want to implement/debug/experiment with another parser than what's in the firmware already.

theflynn49 commented 4 years ago

Lol .. I was writing a joke message about porting xsltproc, but I don't want to mess with your discussion more than I should...

gyurco commented 4 years ago

Well, I more like messing with the FPGA than XML :) And do you know the ARM CPU has 64K RAM? Like the C64.

theflynn49 commented 4 years ago

Let me clarify my position about this Json stuff.

Parsing an INI file requires only one pointer on the param string. Parsing a Json file need one pointer per level of recursion (each "{") you want to support. No more if you don't support silly things like recursive quotes, which are insane. The corresponding code if just a few lines of C and the ram consumption is just limited to a few pointers in the stack.

The advantage it gives you is to facilitate grouping the parameters in a tree structure which is revealed by each value associate by each pointer on the stack during parsing. The RAM cost is very low, the complexity is very low also, assuming to write everything from scratch and you don't use any library, which is implicit in our context.

So if having some kind of "automatic revealing" of the tree tructure of the parameters during parsing has some sense and is of any help in the process, a json structure may be an advantage. If there is no interest whatsoever to structure the parameters in a tree, INI files are ok of course.

Again, my apologies for the digression.

harbaum commented 4 years ago

Still you'd have to put the entire message you want to send into the core somewhere.

How would the current setup benefit from the tree? What config would benefit from that? And more importantly how would you explain to the end user how the file is supposed to look like? Remember: The ini file is human generated.

theflynn49 commented 4 years ago

Param string in temp ram, I must grant this to you (at least after the first pointer).

I don't want to appear like advocating for Json, I hate xsl/xml myself. I just think json is worth considering as a legit format when designing parameter processing (kind of an automatic thinking process of mine), and I was just willing to bring this to your attention. If noone see any interest of having parameters structured as a tree in this project, then of course discard json, it makes everything uselessly complicated. But sometimes, it unlocks interesting possibilities, validations, ease sub-parameters handling, and implementation is not a valid reason to discard it. Being useless is another story. I agree also with the objection that nobody needs yet another param parser in the ARM core.

I really feel sorry for the "out-of-band" discussion, forget all this ;)

harbaum commented 4 years ago

Don't worry. Your contribution is welcome. That's why I ask questions instead of just rejecting your idea. I understand that JSON has it's benefits. Most importantly it allows two machines to communicate in an effective way while still being human readable and expandable.

But in the case of a file that is supposed to be created and maintained by humans I think it has too much pitfalls. Humans will e.g. likely make syntactic mistakes when writing json but the MiST has very limited ways of communicating load errors in the ini file. Users will just complain the that 'it just doesn't work' and we''ll constantly be asking them to upload their ini files somewhere just to tell them that they forgot a closing brace somewhere.

theflynn49 commented 4 years ago

Well, playing devil's advocate here : Strong syntax checking is an advantage in my mind : if your user makes a mistake in an INI file, it won't work either and you won't have a chance to generate an error message. It may even do a lot of silly things because lines of the INI are mixed in a weird way (happened to me several times). This won't happen with a broken json.

harbaum commented 4 years ago

It's imho still easier to write syntactically correct ini files than syntactically correct json files. Yes, broken files are a problem in both cases but one format will likely cause more trouble than the other.

gyurco commented 4 years ago

Well, the ini parser is already written, that's one of the strongest points of that format. And for our purposes, it's OK. I don't think we will ever need a hierarchical format. We couldn't make advantage of it, because we will already out of RAM when the results of such a structure could be used.

theflynn49 commented 4 years ago

all valid points to me.

gyurco commented 4 years ago

Ok, here's the new INI format: Firmware: https://github.com/gyurco/mist-firmware/commit/774f45e0dd716973f6c0b22354feca0ff53c6f7e Scramble examples: https://github.com/gyurco/Mist_FPGA/commit/dd73e5ce111965db22152f4e14daed4193438a50

gyurco commented 4 years ago

Silence is approval then? I've pushed it to mist-devel.

harbaum commented 4 years ago

Sure I am fine with that. Actually I am generally fine with everything you do ....

gyurco commented 4 years ago

Then released a new firmware, and the Scramble example.

jotego commented 4 years ago

I think I understand the format. It should be possible to write a .MRA XML file with the DIP settings and all and then generate a .ARC file via a XSLT (or another transformation tool). I think this can work out well. Please share a binary of the firmware so I can try it out.

gyurco commented 4 years ago

Now there are a couple of ARC-enabled cores: https://github.com/Gehstock/Mist_FPGA_Cores/tree/master/Arcade_MiST/Williams%206809%20rev.1%20Hardware https://github.com/Gehstock/Mist_FPGA_Cores/tree/master/Arcade_MiST/Namco%20Mappy%20Hardware https://github.com/Gehstock/Mist_FPGA_Cores/tree/master/Arcade_MiST/Midway%20MCR%203%20Monoboard https://github.com/Gehstock/Mist_FPGA_Cores/tree/master/Arcade_MiST/Konami%20Scramble%20Hardware

They need the newest firmware: https://github.com/mist-devel/mist-binaries/blob/master/firmware/firmware_200214.upg

jotego commented 4 years ago

Perfect. I will try ARC with CPS core too and if all goes well, I'll cover the old cores to this format too

gyurco commented 4 years ago

Can this be closed then? I think all the shiny things are there now.

jotego commented 4 years ago

I think so. I still haven't tried the DIP option myself but I'll do soon.

El mié., 26 feb. 2020 9:45, gyurco notifications@github.com escribió:

Can this be closed then? I think all the shiny things are there now.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/mist-devel/mist-firmware/issues/42?email_source=notifications&email_token=AAOG27DW2FQWY7NDMNMYCZ3REYT25A5CNFSM4KOCLTOKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEM7KGKY#issuecomment-591307563, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAOG27EI2ULOKWAZ5KOTPYDREYT25ANCNFSM4KOCLTOA .

gyurco commented 4 years ago

Looks like it's handled a bit differently, they're treated a bit special there, here the options are just part of the normal 32-bit status word. This implies you should not use dip bits="0", because it means reset. Maybe you can reserve bits 0-7 for fixed options, and use 8-31 in the MRAs, that will work almost identically.

jotego commented 4 years ago

Yes. I agree. Leaving the first part for fixed MiST settings is a good strategy.

jotego commented 4 years ago

You're right. Sorry I had forgotten about it. I am adapting the code now to work with these specs.