Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

PWM: new implementation #186

Merged
merged 1 commit into from
Jan 7, 2024
Merged

Conversation

vbousquet
Copy link
Contributor

@vbousquet vbousquet commented Jan 7, 2024

This is a rewrite of the preliminary PWM support I added at the beginning of the 3.6 phase. While using it, quite a few limitations arise, so I rewrote it to:

  • Supports variable rate data writing/integration (needed for fast hardware with <1ms strobes)
  • Supports high frequency controller using a linear PWM preintegration applied on high frequency data before physics model (needed for fast hardware with <1ms strobes)
  • Supports strobed/PWM lamp matrix (Stern Whitestar and SAM uses this a lot, but it also improves the rendering on other hardwares)
  • Supports Alphanumeric segment dimming (not yet exposed to VPinMame)
  • Extended to all GI & Solenoids outputs, including aux board (WPC's GI really benefits from this, flashers wired on aux board have been missing)
  • Allow output definition from PinMame driver as well as from client app (allow mods & variants, also allow to support more hardware configuration)

In turn, the rewrite led me to create a sort fo generalized physic output support, then port to it, trying to unify drivers around it. This led to remove (after porting them to the new physic output system) the following implementation:

  • the initial modulated solenoids,
  • the preliminary PWM introduced in 3.6
  • RGB lamps (which were only used by SAM driver to output linearly modulated lights)

This commits includes implementations for the following drivers:

  • WPC,
  • S11,
  • Sega/Stern Whitestar,
  • Stern S.A.M.,
  • Capcom,
  • GTS3

While testing, it appeared that timings were wrong for Capcom hardware, so this commits also contains an improved Capcom driver with better timings (better U16 emulation, fix to MC68k core,...), needed to get correct lamp fading.

This commits also contains the physic hardware definition for the following games: all Capcoms, all WPC, most SAM, GNR for S11, LOTR for Whitestar, none of the GTS3.

The aim would be to have all the physic device connected to binary outputs defined on the PinMame side but it is too much work to be done straight away. Since the implementation allows to define physics device at runtime (for mods & variants), this also allows to define the missing hardware directly from the user application if we do not gather all the info for 3.6.

Using the new implementation is fairly straightforward (at least for games with hardware already defined): just set SolMask(2) = 6, then changed lamps / solenoids / gi will be 0..255 value relating to the main characteristic of the output (relative brightness for bulbs & LEDs).

Since this is a deep change, I think there will be bugs I jave missed, so this would benefit for large user testing. Also, @mkalkbrenner @jsm174 I tried to keep things clean for PPUC and LibPinMame (and PROC) but since I don't use them, the testing is fairly low.

@jsm174
Copy link
Contributor

jsm174 commented Jan 7, 2024

Wow, this is amazing! From the looks of it, I think all I need to do is modify

https://github.com/vpinball/vpinball/blob/66942f17ceebf2ff0badec6d94431cf12a584f2d/standalone/inc/vpinmame/VPinMAMEController.cpp#L1071-L1089

and I should be good on the standalone side.

@vbousquet
Copy link
Contributor Author

There is another thing to be consider for LibPinMame: you are using OnSolenoid events. I just ported the code for it but I think this could be adapted to benefit for the change, for example by directly processing events for SOL_2_STATE while delaying the other to VBlank (as they are now). This should improve the latency quite a lot if event are directly processed

@vbousquet
Copy link
Contributor Author

I forgot to share the updated core.vbs for VPX that ease using this (I will commit it to VPX when VPM will have th efeature integrated):
core.zip

Supports variable rate data writing/integration
Supports high frequency controller using a linear PWM preintegration applied on high frequency data before physics model
Supports strobed/PWM lamp matrix
Supports Alphanumeric segment dimming (not yet exposed to VPinMame)
Extended to all GI & Solenoids outputs, including aux board
Allow output definition from PinMame driver as well as from client app

This commits includes implementations for the following drivers: WPC, S11, Sega/Stern Whitestar, Stern S.A.M., Capcom, GTS3
This commits also contains the physic hardware definition for the following games: all Capcoms, all WPC, most SAM, GNR, LOTR

This commits also contains an improved Capcom driver with better timings, needed to get correct lamp fading.
@toxieainc toxieainc merged commit dbf8888 into vpinball:master Jan 7, 2024
32 of 36 checks passed
@toxieainc
Copy link
Member

Great!
One more thing: Could you please generalize the game name detections some more? Otherwise just the newest ROM revisions will work correctly i guess..

@toxieainc
Copy link
Member

..oh, and some text for the whatsnews, please.. Thanks!

@vbousquet
Copy link
Contributor Author

Whatsnew update (directly in commit, I force pushed it)
The game name detection is already generalized: it walk up to the root game name and compares against it, therefore handling all the clones at once.

@vbousquet
Copy link
Contributor Author

vbousquet commented Jan 7, 2024

Ooopss, you were fast :) my force push was after the merge.
I will do a new PR for the whatsnew

@toxieainc
Copy link
Member

The game name detection is already generalized: it walk up to the root game name and compares against it, therefore handling all the clones at once.

Really? Only GTS3 seems to be, but what about these?

if (strncasecmp(gn, "gnr_300", 7) == 0) { // Guns'n Roses

if (strncasecmp(gn, "afm_113", 7) == 0) { // Attack from Mars

if (strncasecmp(gn, "abv106", 6) == 0) { // Airborne

if (strncasecmp(gn, "acd_170h", 8) == 0) { // AC DC

@volkenborn
Copy link
Contributor

volkenborn commented Jan 8, 2024 via email

@vbousquet
Copy link
Contributor Author

vbousquet commented Jan 8, 2024

The game name detection is already generalized: it walk up to the root game name and compares against it, therefore handling all the clones at once.

Really? Only GTS3 seems to be, but what about these?

I may have miss something. The aim of this code (which is present for all drivers) is to move to the root gamedef:

  const struct GameDriver* rootDrv = Machine->gamedrv;
  while (rootDrv->clone_of && (rootDrv->clone_of->flags & NOT_A_DRIVER) == 0)
     rootDrv = rootDrv->clone_of;
  const char* const gn = rootDrv->name;

Then I use the name of the root game definition. I don't see the difference with GTS3.

@vbousquet
Copy link
Contributor Author

Hi guys, this change in core.c will lead to a lamp matrix 8 times the size as it was before

It's a bug! Thanks for spotting, I will remove the * 8 in int nCols = coreGlobals.nLamps ? (coreGlobals.nLamps + 7) >> 3 : CORE_CUSTLAMPCOL + core_gameData->hw.lampCol * 8;

@toxieainc
Copy link
Member

The game name detection is already generalized: it walk up to the root game name and compares against it, therefore handling all the clones at once.

Really? Only GTS3 seems to be, but what about these?

I may have miss something. The aim of this code (which is present for all drivers) is to move to the root gamedef:

  const struct GameDriver* rootDrv = Machine->gamedrv;
  while (rootDrv->clone_of && (rootDrv->clone_of->flags & NOT_A_DRIVER) == 0)
     rootDrv = rootDrv->clone_of;
  const char* const gn = rootDrv->name;

Then I use the name of the root game definition. I don't see the difference with GTS3.

Whoops, you're completely right. Sorry for the noise.. :/
Can you then please commit the lamp matrix change, cause then i could start the next PinMAME beta.. Thanks!

@vbousquet
Copy link
Contributor Author

Can you then please commit the lamp matrix change, cause then i could start the next PinMAME beta.. Thanks!

It's already in there: #188

Regarding engaging the beta, I had a look yesterday evening to dimmed segment (for GTS3 and WPC) and it seems more simple than I expected. I implemented it for GTS3, just hadn't time to do the full thing (only the driver part for now, not the PinMame display, and VPinMame interface). Also, I finally had access to a bunch of Gottlieb schematics which will allow to add some definitions of physics outputs to GTS3 tables. So, perhaps wait until this week-end for the beta

@toxieainc
Copy link
Member

Oh my, seems my brain is still in xmas mode ;)
Thanks!

Then i'll also wait some more, cause this sounds like an extremely cool feature to have..

@mkalkbrenner
Copy link
Contributor

@vbousquet thanks for working on this and for pinging me.
I'll test things with PPUC, but it will take some time because we focused on WPC during the last months. To test SAM we need to "configure & wire" a test case first.
@toxieainc so don't wait for my feedback and go ahead.

@vbousquet
Copy link
Contributor Author

vbousquet commented Jan 12, 2024

I'll test things with PPUC, but it will take some time because we focused on WPC during the last months.

The new implementation replace modulated solenoids that were implemented for WPC and SAM, so it changes WPC as well.
Note that it also adds an integrator that may be of interest for PPUC to drive LEDs from old controller logic (to get correct brightness but also you could use it to drive RGB LED with the color computed from filament temperature).

@vbousquet
Copy link
Contributor Author

vbousquet commented Jan 12, 2024

@toxie dimmed segments added. They seem to fully work in PinMame but there is no API for this in VPinMame, so they are not available on VPX / B2S hwich is a bit sad.

I also modified the implementation to only update outputs on request (very very basic lazy multithreading support) since the load was a bit heavier with the segments (there are a lot of them).

@jsm174 this may cause isssue with libpinmame since I don't know the threading model of LibPinMame, therefore I did not do anything and it just rely on the default calls to core_update_pwm_outputs. Perhaps you may need to change this (either call core_request_pwm_output_update if you are multithreaded or directly call core_update_pwm_outputs when needed if single threaded but at a pace corresponding to the client, not the default).

And forgot to say, it's in here: #190

@jsm174
Copy link
Contributor

jsm174 commented Jan 12, 2024

@vbousquet, for vpx standalone, there's a runner thread, but everything else should work like the normal controller.

So I'm guessing I will need to just call core_request_pwm_output_update() in

https://github.com/vpinball/pinmame/blob/505d906a1f6d113f646007c85eddd8f32bc06cb8/src/libpinmame/libpinmame.cpp#L1035C16-L1038

and do the same for all the other functions?

@toxieainc
Copy link
Member

Great stuff!
What do you recommend as API for VPM?

@vbousquet
Copy link
Contributor Author

vbousquet commented Jan 13, 2024

So I'm guessing I will need to just call core_request_pwm_output_update() in....

Yes, this is what schedules the update. So all calls coming from another thread should call it (it is thread safe).
You will also need to start the timer in core.c (it is #ifdef to VPINMAME only for the time being)

This design has the benefit of nearly 0 synchronization cost but the drawback that the data returned is always late (the one updated asynchronously from the last request, except for outputs that do not perform integration at all like 2 state solenoids). If the update request is not called regularly, this may lead to not perfectly smooth fading. I tested it and it never happened to me so I kept this since it lower the CPU load (16 times lower) but if we find issue, we can get back to the systematic update every 1ms (the CPU load with this is still around 5% for a 10 year old computer so... but with the 640 alpha segments of a GTS3 around 9% of these 5% CPU time is spent in the output integrators).

Great stuff!

Thanks! this needs to be merged though: #190

What do you recommend as API for VPM?

My feeling is that the arch and the API needs to have a global evolution since the way we interface VPX and VPM has the following drawbacks from my point of view:

  • it creates high latency (no callbacks, B2S in the way using Windows registry for communication,...)
  • it uses low precision bytes where floats would be needed
  • it freezes the API and hinder progresses since every evolution of VPM needs an evolution of VPX, B2S and often tables, all of this needing to be more or less coordinated
  • it splits the output in predefined groups (solenoids/LEDs/GI/lamps) that in fact do not fully match the reality (you have bulbs, motors,... in solenoids, you have LEDs in lamps,... GI is made of lamps and sometimes like CFTBL has controlled lamps like chase lights,...)

So, we have a few options here:

  • either do the full thing: create the module/plugin arch that will allow to normalize standalone modules as plugins/modules on all platform, move VPM to one of these using LibPinMame, use standalone B2S implemntation as one of these plugins, keep B2S (or a module) just for its plugin, or even just port the plugins as new modules... We could also have a controller object, hiding the API and allowing easier evolution (to avoid tables needing to be updated). This is huge and will take a looonnngggg time, so definitely not for 10.8
  • don't do anything (so no dimmed segments)
  • change the existing API call to return different values depending on physics output being enabled or not => this would need B2S to be updated since it processes this data as well => not a good idea since it makes the API more complex (hacky ?) and needs VPX+VPM+B2S update
  • create a new API call, update B2S for it & update core.vbs => this can be done, but this will postpone one more time 10.8 (not a real problem, but at some point, it would be nice to move on) and will create an API call that will be deprecated if we do the full thing
  • report the alpha segment as lamps using a dedicated range => this would likely work as-is on VPX-VPM-B2S side but this means a new 'hack' on the table side, which is the most difficult to coordinate when things will evolve again (we could use one of solenoid, gi or lamps to report all physic outputs)

@volkenborn
Copy link
Contributor

volkenborn commented Jan 16, 2024

Hi again,

now in the console version all solenoids numbers logged to screen have a value 1 less, so sol#1 will write out "0", sol#2 will write "1", and so on.

This is due to a change in core.c, lines 1500 and 1532 (ii starts with a value of 0, before it was 1).
I suppose we can change it back to 1, and call OnSolenoid without the extra increment?!

Or, alternatively, just put:
locals.solLog[locals.solLogCount] = ii + 1

and
proc_mechsounds(ii + 1, ...)

in both cases.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants