update to add network for Sega X-Board, Y-Board, Konami K056230, Namco C139 #13421
Conversation
make "connection accepted" to be distinguishable from "end-of-stream"
xbdcomm: added comm board for "Last Survivor" and "Super Monaco GP" ybdcomm: added comm board for "Power Drift - Link Version"
k056230: add network to 'GTI Club', 'Jet Wave', 'Midnight Run', 'Polygonet Commanders', 'Poly-Net Warriors' and 'Winding Heat'
harddriv: map seat axis. gticlub: output drive commands. zr107: output drive commands.
- 'Driver's Eyes' now working - other C139 games are working but network unstable.
|
pdriftl - https://www.youtube.com/watch?v=R8OlbqSLZhQ |
- removed C139_SIMULATION define altogether - added state saving where needed - set default values on device_reset - removed duplicate variable definitions - inconsistent hexadecimal-constant capitalization should be fixed now - function names converted from camelCase to snake_case - every switch should now have a default case and a final break; - removed MACHINE_NODEVICE_LAN where applicable, left it in place where link is unstable (most namco c139 cases) k056230: use short types (u8) instead of full types (uint8_t) to stay consistend within existing code gticlub: thunderh / thunderhu don't use the LANC at all? removed
|
All or nearly all of my feedback points have been addressed, it doesn't appear that I can mark them as resolved on my own, though. |
To be honest I can't even see any of those on github. |
|
I'm okaying decision to remove "NODEVICE_LAN" flag from thunderh/thunderhu because of its LANC usage for something other than cabinet linking. |
There was a problem hiding this comment.
I’ve only looked at a few of the files, but I’ve already found a bunch of stuff. In addition to the in-place comments, quite a few files seem to be missing the terminating newline – you might want to run MAME’s srcclean tool over the files you’ve changed to clean that up as well as other formatting issues.
I expect at least some of the things I’ve noticed in the files I looked at already will be applicable to multiple files, so if you could apply fixes to other applicable places preemptively it might expedite things a bit.
src/mame/sega/m1comm.cpp
Outdated
| std::error_condition filerr; | ||
| std::uint32_t written; | ||
|
|
||
| filerr = m_line_tx->write(&m_buffer0, 0, dataSize, written); | ||
| uint32_t written = 0; | ||
| std::error_condition filerr = m_line_tx->write(&m_buffer0, 0, dataSize, written); | ||
| if (filerr) | ||
| { | ||
| osd_printf_verbose("M1COMM: tx connection error\n"); |
There was a problem hiding this comment.
Since you’ve got a non-blocking socket, there’s a very real possibility that you’ll end up with a partial write here. You should be checking written. Blocking really isn’t an option, as you’ll hold up the emulation. Have you thought about what to do in that situation? (An asynchronous signal can also cause a partial write to happen.)
Also remember that you won’t get an error from a partial write, you need to attempt to complete the write to see the error. That’s the reason for those read/write loop helpers in src/lib/util/ioprocs.cpp
There was a problem hiding this comment.
Are you sure we are using non-blocking sockets here?
As far as posixsocket.cpp and winsocket.cpp go, sockets are opened with TCP_NODELAY (which disables nagle's algorythm)
posixsocket.cpp sets O_NONBLOCK for "domains" only
There was a problem hiding this comment.
Sorry, I was getting confused with the sane way of doing things. MAME does a select every time, doesn’t it. That means it can end up blocking if the amount of data available is less than the buffer size you pass in. There are so many pitfalls in MAME… But trying to sort that out is a job for another time.
Anyway, you still need to be aware that a write may partially complete for various reasons, and if an error occurs after a write partially completes, you need to call write again to attempt to complete the write before you’ll see the error.
See #11608 for more explanation of semantics.
There was a problem hiding this comment.
should be fine now.
Did you remember to hit the “complete review” button or whatever it’s called? |
- reorganised most includes. - made ybdcomm mem/io maps private. - removed several comments about device-level overrides - added dip switch locations for ybdcomm and removed invalid options. - replaced digit clock speeds by XTAL markup. - changed localhost/remotehost handling to strings with util::string_format. - added side_effects_disabled-checks to memory/io reads. - changed most debug logging to use LOG macro. removed others. - socket errors now output error code and message. - moved variable declarations around to keep them in the smallest scope possible.
|
Sorry, almost 3AM, too tired to look at the full diff again tonight. I’ll get back to it some time tomorrow. |
- added more detailed error logging. - handle partial reads/writes in a more elegant way.
I don't even see a "Complete Review" button, which is bizarre as I certainly seemed to be able to open one. |
src/devices/machine/mb89372.cpp
Outdated
| { | ||
| m_channel[channel].m_state = 1; | ||
| m_current_channel = channel; | ||
| logerror("meep... %01x\n", channel); |
There was a problem hiding this comment.
Could this message be made more informative?
There was a problem hiding this comment.
to be honest,
I don't even remember why I added that output in the first place.
I wasn't able to source a comm board with that chip to do tests on the real chip.
I'll remove that output.
There was a problem hiding this comment.
I’m really not comfortable with stuff that can easily get into a state where it makes MAME unresponsive to user input. No-one likes software that freezes and needs to be killed when something goes wrong. In this case, something going wrong could be as simple as misconfiguration causing connections to time out, loss of network connectivity, or the name service becoming unresponsive.
Now taking a step back, you’re up against framework issues:
- Saying the
osd_fileinterface was “designed” is giving it too much credit. Why is getting the length (which may not be applicable) part of opening a file? Why do reads and writes always have an implicit seek (which also may not be applicable)? Sockets hit both of those. - The “magic” file names that create sockets were really just intended to do things like connecting a serial or parallel port to a socket rather than a file for dumping output. They connect or listen on open (which happens on start or when you change media), and they don’t give much control (e.g. can’t cancel blocking operations, read will block if data is available but smaller than buffer size passed in, etc.).
You can mitigate the issues somewhat by moving socket operations (or at least opening socket) to a worker thread. But even if you do that, you have no way to cancel a connection attempt, which means the user could end up having MAME freeze for over a minute waiting for a connection to time out so you can clean up your worker thread if they happen to try to exit just after a connection attempt begins.
Have you considered using ASIO directly for your networking code like the CPS2 comm board (for SF2: The Tournament Battle)? That gives you asynchronous operations (emulated using threads if necessary), and you get a lot more control as you don’t have sockets hiding behind an interface that’s barely adequate for random access files.
src/devices/machine/k056230.cpp
Outdated
| // check rx socket | ||
| if (!m_line_rx) | ||
| { | ||
| osd_printf_verbose("k056230: rx listen on %s\n", m_localhost); | ||
| u64 filesize; // unused | ||
| std::error_condition filerr = osd_file::open(m_localhost, OPEN_FLAG_CREATE, m_line_rx, filesize); | ||
| if (filerr.value() != 0) | ||
| { | ||
| osd_printf_verbose("k056230: rx connection failed - %s:%d %s\n", filerr.category().name(), filerr.value(), filerr.message()); | ||
| m_line_rx.reset(); | ||
| } | ||
| } | ||
|
|
||
| // check tx socket | ||
| if ((!m_line_tx) && (m_txmode == 0x01)) | ||
| { | ||
| osd_printf_verbose("k056230: tx connect to %s\n", m_remotehost); | ||
| u64 filesize; // unused | ||
| std::error_condition filerr = osd_file::open(m_remotehost, 0, m_line_tx, filesize); | ||
| if (filerr.value() != 0) | ||
| { | ||
| osd_printf_verbose("k056230: tx connection failed - %s:%d %s\n", filerr.category().name(), filerr.value(), filerr.message()); | ||
| m_line_tx.reset(); | ||
| } | ||
| } |
There was a problem hiding this comment.
Wouldn’t it make sense to start listening on start rather than in an address space write handler?
I’d question whether it’s a good idea to try opening sockets in a write handler at all:
- Opening a socket calls
gethostbynamewhich blocks on name service lookup. This can be time-consuming, and you’re holding up the emulation with this. - At a quick glance, it appears to use a synchronous
connectcall, which is also going to hold up emulation, particularly if there’s a timeout (this could cause the UI to become unresponsive for over a minute – the default timeout is 72 seconds on Windows). - You aren’t keeping track of the wall clock time on a failed connection attempt, and you aren’t implementing any kind of back-off on failed attempts.
The net result is that this approach is liable to cause MAME to get into a state where it appears to be frozen, and a user is unlikely to be holding the key/button assigned to UI Cancel in the brief interval between the long blocking connection attempts, so it will look like you can’t exit.
| while (bytes_total < data_size) | ||
| { | ||
| std::error_condition filerr = m_line_rx->read(&m_buffer0[bytes_total], 0, data_size - bytes_total, bytes_read); | ||
| if (filerr) | ||
| { | ||
| bytes_read = 0; | ||
| // special case if first read returned "no data" | ||
| if (bytes_total == 0 && std::errc::operation_would_block == filerr) | ||
| return 0; | ||
| } | ||
| if ((!filerr && bytes_read == 0) || (filerr && std::errc::operation_would_block != filerr)) | ||
| { | ||
| osd_printf_verbose("k056230: rx connection failed - %s:%d %s\n", filerr.category().name(), filerr.value(), filerr.message()); | ||
| m_line_rx.reset(); | ||
| return 0; | ||
| } | ||
| bytes_total += bytes_read; | ||
| } |
There was a problem hiding this comment.
Won’t this spin and hold up emulation if it receives a partial frame? Spinning and making MAME unresponsive waiting for network data that may not arrive is not going to provide a pleasant user experience. If you get a “would block” error, you need to let the emulation continue and check for data again later.
| while (bytes_total < data_size) | ||
| { | ||
| std::error_condition filerr = m_line_tx->write(&m_buffer0[bytes_total], 0, data_size - bytes_total, bytes_sent); | ||
| if (filerr) | ||
| { | ||
| osd_printf_verbose("k056230: tx connection failed - %s:%d %s\n", filerr.category().name(), filerr.value(), filerr.message()); | ||
| m_line_tx.reset(); | ||
| return; | ||
| } | ||
| bytes_total += bytes_sent; | ||
| } |
There was a problem hiding this comment.
As I said on a previous iteration, you really don’t want to be holding up emulation waiting to send data. You want to defer sending the remaining data until a point where you won’t get blocked.
|
While I see your points, I have to say I am getting really frustrated. YES, there are some possibilities to "freeze" MAME momentarily. That being said however... The code for listening and connecting is there by design. The idea being that the "other" instance might not be started yet, hence multiple tries - or the the user is reseting via F3. Also, the listening socket stops listening after accepting one connection. The read loop has been there (at least in my code) since the beginning, as fragmented packets should not be happening in the first place. The newly added write-loop got there, because you requested me to handle partial writes that "might" appear. Both loops will instantly exit as soon as the socket returns any error (besides EWOULDBLOCK on partial reads) The code is blocking by design, there even is - at least on some comm implementations - an option implemented to "framesync" the emulation. this is done by deliberately spinning until a special frame has been received or an error occured. I know that my code probably is not be the "best" way of doing things. But it is all that I have for now - and it is working. I buy boards, "borrow" chips and reverse engineer them by building hardware contraptions to hook them up to an arduino or an PC via ISA-bus. Right now I am waiting for a pcb to arrive so I can hook up a real C139. I spend ages single stepping through code that reads/writes to a chip and try to figure out what the code is expecting and how everything is supposed to be working. Then I come up with some emulation, and refine that, until it is "working". The first code for that (m1comm) has been that way almost unchanged since I came up with it in 2012. Most of that stuff has been running for several years now on my cabinets, as seen on my youtube channel. I'll happyly add a "TODO: make stuff non blocking" in there, but most certainly will not rewrite the core logic of it right now. -- As we are talking about user experience... what is the typical use case? The normal user won't use the COMM stuff in the first place. So the default of connecting to itself (via 127.0.0.1) should not be an issue. The advanced user that wants to use the COMM stuff to link up several instances will probably do that on the local machine, or on the local network. That same user likely will know that configuration or connection issues will cause trouble. -- EDIT -- Assuming the remote machines stay "on"...
unpleasent, yes - but no need to fire up task manager and "kill" mame. If the other machine simply turns off, hence not closing the socket gracefully, the last instance will freeze for as long as the timeout for TCP sockets is set. Like I previously mentioned, linking multiple mame instances is a rather advanced topic in the first place. |
- add a "delay" between connect attempts in k056230. (usually 1 video frame) - removed obsolete logerror in mb89372 - noted that C422 and C139 are pin combatible.
|
The ASIO stuff in cps2comm seems simple enough. |
|
I want to throw my two cents in on this whole "OSD vs. ASIO" thing: Given the fact that @SailorSat here isn't just the person developing these networking-board implementations, but is the person actively using them, she knows better than anyone whether @cuavas's assertions of doom and gloom about main-thread blocking are likely to happen given the workflow of a typical user of this feature. This is a feature intended not for playing these games across the Internet, but across multiple instances of MAME on either the same machine or on a LAN, for local parties and such. The concerns being expressed about "what happens if a connection drops out" are broadly irrelevant, because the feature already self-selects for users that know what they're doing. Average users are almost certainly going to try to use it across the Internet, and they'll end up disappointed no matter what Ariane here does - because a user trying to do that is one of those "Doctor, it hurts when I hit myself with this hammer" situations. The more likely scenario is temporary stutter or blocking of the main thread, if it can be blocked at all. When MAME already becomes unusable on Windows if the user erroneously fullscreens MAME while running with Stop holding up contributors' progress in other areas of MAME just because there are some sins in the core that could and should be rectified. With the nebulous-to-nonexistent timeline for addressing those issues, it comes off as hypocrisy given the history of giving grief to @smf- for purportedly doing the same. |
- stupid me.
- hooked up outputs for system22 games
This includes updated socket handling - accepting an incoming socket could be mistaken for end of stream; this has been changed so that accepting returns a would_block error.
Sega X-Board comms make 'Super Monaco GP', and 'Last Survivor' playable in LAN.
Sega Y-Board comms make 'Power Drift - Link Version' playable in LAN (setup dips correctly!)
Konami K056230 makes 'GTI Club', 'Jet Wave', 'Midnight Run', 'Polygonet Commanders', 'Poly-Net Warriors' and 'Winding Heat' playable in LAN.
Namco C139 makes 'Driver's Eyes' working
other C139 games are working but the network is highly timing critical (unstable).
--- image spam below ---
Konami K056230
Namco C139
