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

bus/a2bus: Added Uthernet card emulation for Apple IIgs #7090

Merged
merged 18 commits into from Aug 31, 2020

Conversation

roadriverrail
Copy link
Contributor

@roadriverrail roadriverrail commented Aug 16, 2020

Note to the MAME/MESS team: I was working with GSPort over the weekend and found it wasn't handling certain 2mg files correctly. Rather than try to fix 2mg, I decided it would be easier to port GSPort's "The Final Ethernet" system for emulating Uthernet cards. Because TFE relies on libpcap (and thus on the ability to operate in promiscuous mode), this is likely not an ideal solution. Additionally, I can't really vouch for how it'd work across platforms at this time. However, the Uthernet emulation should work fine, so I thought I'd see if anyone on the MAME/MESS teams would like to help me flog this code into shape as a feature.

(Original commit message below)

On the GSPort emulator, the Uthernet Ethernet card is emulated using The Final
Ethernet (TFE), which appears to have been originally written for a C64 emulator
and adapted to GSPort. This commit ports TFE use into the MAME/MESS system
enabling Ethernet on the MESS Apple IIGS emulator.

To use:

  • Compile from source
  • MESS must be run as root, must be SUID root, or must have the cap_net_raw and
    cat_net_admin capabilities set (ala Wireshark)
  • Run the IIGS emulator with -sl3 uthernet -uthernet_intf , where
    is an Ethernet interface name from "ifconfig"
  • Configure Uthernet as usual

Note this expects slot 3 is Uthernet and you must use an Ethernet interface
for the underlying interface.

@rb6502
Copy link
Contributor

rb6502 commented Aug 17, 2020

Wow! Yeah, this definitely can't go in as-is, but it's a very exciting pull request regardless. I've had A2 Ethernet in the back of my mind recently after noticing that the LANceGS card uses an Ethernet chip we already support in MAME. The Uthernet seems to have more widespread software support though, and none of the hassle with the "copy protection string" in the card's non-volatile storage.

Anyway, MAME has an existing Ethernet layer that uses TAP/TUN on Linux and pcap on Windows/Mac. Working with that would be an ultimate requirement. But before that, it looks like there's a substantial amount of non-Ethernet-related GSPort code in this submission that should be removed - I see IWM and ADB references just skimming.

Copy link
Member

@cuavas cuavas left a comment

It would be nice to support the card in principle, but it needs to be ported to use device_network_interface (from src/emu/dinetwork.h) rather than its own direct bindings to the pcap library. (I’m not necessarily implying that @roadriverrail needs to make these changes, just flagging the reason the pull request can’t be merged in its current form.)

@cuavas
Copy link
Member

cuavas commented Aug 17, 2020

Anyway, MAME has an existing Ethernet layer that uses TAP/TUN on Linux and pcap on Windows/Mac.

FWIW we support TAP/TUN on Windows and pcap on Linux as well. We don’t build in pcap support by default on Windows (due to licensing issues) or Linux (to reduce out-of-the-box dependencies).

@roadriverrail
Copy link
Contributor Author

roadriverrail commented Aug 17, 2020

@rb6502 and @cuavas thank you both for your feedback. I didn't actually make the PR with immediate intent to merge so much as to see if there was interest in work proceeding here. I'm no slouch, but I'm simply under-educated about the MAME architecture and wanted to see if there'd be interest in proceeding on this front. If I can get enough support with people pointing me in the right steps to making this feature-worthy, then I'll personally take on a significant amount of the grunt work. At the end of the day, it's a feature I want for myself, and I'd rather see it in MAME than in my private repo where I have to do a lot of pick-up rebases to keep it current.

@cuavas
Copy link
Member

cuavas commented Aug 17, 2020

@roadriverrail
Copy link
Contributor Author

roadriverrail commented Aug 17, 2020

Thanks. I'm doing a basic code scrub right now, but I think once I'm done I'll see about a set of steps to make TFE small enough to fold into the uthernet.cpp source and then it's basically a question of replacing the PCAP stuff with your APIs.

@rb6502
Copy link
Contributor

rb6502 commented Aug 17, 2020

That sounds great, with the caveat that the TFE portion (more accurately the CS8900A chip) would need to be a separate device, like the other Ethernet chips @cuavas pointed to. That way we can also use it for other systems that had CS8900A-based adapters.

ETA: Here's a link to the programming manual for the CS8900A.
http://mirrors.apple2.org.za/Apple%20II%20Documentation%20Project/Interface%20Cards/Network/a2Retrosystems%20Uthernet%20Card/Datasheets/

@roadriverrail
Copy link
Contributor Author

roadriverrail commented Aug 23, 2020

Hey, all. So, I've made progress towards a separate CS8900A networking device and have the Uthernet device relying on it. I've reached the point where I'm seeing the register read/writes and, I think, even seeing the MAC address getting set. So, I'm definitely moving on to "live fire testing".

The only documentation I've seen on setting up and using the MAME/MESS tuntap system was in the documents for the Apollo emulator: https://wiki.mamedev.org/index.php/Driver:Apollo#How_to_use_the_MAME_Network_Device

First off, is this the only documentation on the use of the utility? It looks very case-specific. Additionally, it looks like there's a presumption of static IP assignment to the emulator. Previously, I'd been letting Marinetti's DHCP client get an address from the home router directly, so I'm guessing that's not supported and I need to set up a fixed address assignment on my home router? Finally, while I've played with tun/tap before, I'm not 100% familiar with it. Will the raw frames I'm sending appear unmolested on the wire, MAC and all? This can matter for how I do my test setup, because multiple MACs appearing on a wifi interface can lead to the packets getting dropped (TFE previously only worked on Ethernet for this reason).

Thanks for the guidance so far. I suspect I'm only a few labor hours from a working demo, and I can update the PR at that time, though I will then go on a campaign of extensive code cleanup.

@rb6502
Copy link
Contributor

rb6502 commented Aug 23, 2020

That sounds great! The actual taputil.sh usage is emulation-independent; I've used the same procedure for the PC and 68k Mac drivers. I think you can use DHCP even if you set an IP in the TAP/TUN configuration, but I've just used static personally. @cracyc might have additional insight here.

@ksherlock
Copy link
Contributor

ksherlock commented Aug 23, 2020

GSPort's TFE was borrowed from VICE. VICE later cleaned up their code and renamed it to rawnet and split out cs8900 into a separate file which is probably a better starting point.

https://sourceforge.net/p/vice-emu/code/HEAD/tree/trunk/vice/src/core/cs8900.c
https://sourceforge.net/p/vice-emu/code/HEAD/tree/trunk/vice/src/core/cs8900.h

@MooglyGuy
Copy link
Contributor

MooglyGuy commented Aug 23, 2020

To add to the above, I've looked into it and VICE is distributed under GPLv2+, which is in fact compatible with our licensing.

@roadriverrail
Copy link
Contributor Author

roadriverrail commented Aug 23, 2020

@ksherlock and @MooglyGuy thanks for that! The TFE code I'm working off of is indeed exceedingly ugly. I think what I have may be functional, so after some functionality testing, I'm going to look at the cs8900a stuff from them and drag that over rather than go through my own rewrite/beautification process.

@pmackinlay
Copy link
Contributor

pmackinlay commented Aug 24, 2020

@roadriverrail nice to see you're progressing with adding this hardware to MAME, network all the things!

The OSD TAP driver for Linux expects to find a TAP adapter named tap-mess-%d-0, where the %d is the value returned from getuid(), so that aspect is fixed. On Windows, you can create any TAP adapter you like and you must then select the one you want to assign through the in-emulation menu or configuration.

The raw frames that you generate starting with the destination MAC and ending with the FCS will be sent onto the TAP adapter "wire". The source MAC appears outside as expected; one of the first signs of life is the emulated system MAC showing up in the ARP cache on your host.

On the receive side, the OSD TAP driver does some small amount of additional work - padding the received data to a minimum length of 60 bytes and then computing and appending an FCS before passing the result to the device. This is needed because TAP often receives frames shorter than the minimum length (the OS expects the adapter to provide padding), and without the FCS.

Given this, it's expected that in your own code you will both generate and check/remove the FCS on your outbound and inbound frames respectively (as well as doing your own unicast/multicast/broadcast filtering, etc.).

The TAP adapter is independent from the other physical adapters in your system, and you have the choice to route or bridge its traffic elsewhere. If you choose to route the traffic, it's controlled using standard routing/filtering tables and similar, so you shouldn't have any issues with IPv6 or Wi-Fi chatter etc., other than perhaps some unwanted traffic that your emulated system should happily ignore. Unfortunately, DHCP isn't routable, so you'd be limited to running the DHCP server on your host to service the requests from the TAP network in this case, but that configuration should work.

If you bridge the TAP adapter, you may encounter some issues such as you described, and I'm not sure if you can make dynamic addressing work easily in a bridged configuration. If you statically configure your addresses (or if you use something similar to the Internet Connection Sharing service on Windows, which has an integrated mini-DHCP service within its bridging function), you should be able to get regular traffic in and out successfully.

I have found static address assignment and routing to be the simpler option, and for me logically it makes some sense to treat the emulated subnet differently, not least due to the big mismatch in speeds between old (often 10Mbps or less) emulated adapters and modern network equipment.

@roadriverrail
Copy link
Contributor Author

roadriverrail commented Aug 24, 2020

@pmackinlay Thanks for the extra advice. Between this an those docs for setting up Apollo networking, there's been good progress here. I just got done confirming a telnet connect sequence between the Apple IIGS emulator and my workstation! When I attempted a connection to my LAN telnet BBS at a different IP address on the same subnet, though, I could see TCP SYN packets on the tap device (using Wireshark) but no traffic to the appropriate IP address was found on my actual Ethernet interface. Given that I can make a TCP connection between the IP addresses of the emulator and the workstation controlling the tap interface, though, I suspect this is an issue of IP routing? Let's say the emulator is 192.168.128.58, the workstation is 192.168.128.40, and the LAN BBS is 192.168.128.58. I did a little digging before asking you, but was having trouble making sense of it.

Outside of this, though, this was a successful test of the CS8900A code and Uthernet emulation; the emulator's putting packets on the wire and pulling them off the wire!

@roadriverrail
Copy link
Contributor Author

roadriverrail commented Aug 24, 2020

@pmackinlay I ultimately was able to fix it with iptables doing a little NATting for me:

rhett@downhome:~/mame$ sudo iptables -A FORWARD -i tap-mess-1000-0 -o enp0s31f6 -s 192.168.128.58 -j ACCEPT
rhett@downhome:~/mame$ sudo iptables -A FORWARD -i enp0s31f6 -o tap-mess-1000-0 -d 192.168.128.58 -j ACCEPT
rhett@downhome:~/mame$ sudo iptables -t nat -A POSTROUTING -o enp0s31f6 -s 192.168.128.58 -j MASQUERADE

So, this feels okay to live with, though maybe we want to include something like this in the taptun.sh script? I'd imagine this would be relevant to users wanting to network/BBS from an emulator. Again, I'm happy to take on the effort of writing more documentation and so forth.

@ksherlock I gave the VICE cs8900a code you linked a little look-see, and I think the difference between it and what I've dragged in from GSPort's TFE is mostly cosmetic. Since I've got something working and they don't look functionally different, I think I'll beautify what's already working, though once I'm done I think the credit will go to VICE, and I'm glad to have the original authors current details (and license) to attribute.

@rb6502
Copy link
Contributor

rb6502 commented Aug 24, 2020

I didn't need to do any fancy routing to access other machines on my subnet (in particular, I can file-share between an emulated Mac in MAME and my PowerMac G5 running a very old OS X), but I completely disabled firewalld on my system.

On the GSPort emulator, the Uthernet Ethernet card is emulated using The Final
Ethernet (TFE), which appears to have been originally written for a C64 emulator
and adapted to GSPort.  This commit ports TFE use into the MAME/MESS system
enabling Ethernet on the MESS Apple IIGS emulator.

To use:
* Compile from source
* MESS must be run as root, must be SUID root, or must have the cap_net_raw and
  cat_net_admin capabilities set (ala Wireshark)
* Run the IIGS emulator with -sl3 uthernet -uthernet_intf <interface>, where
  <interface> is an Ethernet interface name from "ifconfig"
* Configure Uthernet as usual

Note this expects slot 3 is Uthernet and you *must* use an Ethernet interface
for the underlying interface.
This commit removes the 3rdparty "TFE" library dependency and moves the
logic into device classes to emulate enough of the cs8900a to support the
Marinetti TCP/IP stack for the apple2gs.
@roadriverrail
Copy link
Contributor Author

roadriverrail commented Aug 25, 2020

I've dropped the TFE library from the 3rdparty and introduced cs8900a as a device, with the uthernet card relying on it. Worked well enough in a few Marinetti telnet sessions and it's far more attractive. Does the MAME team keep an uncrustify config file? A style guide? If not, I'll just welcome rolling red ink. I started with some rather unpretty stuff, so please don't take this as par for my personal style.

@cuavas
Copy link
Member

cuavas commented Aug 25, 2020

If you have no strong feelings on coding style, we recommend using Allman style (one tab indent per level of scope, curly braces on their own lines).

In general:

  • Be consistent within a compilation unit – choose a style and stick to it
  • snake_case for class names, enum names, free function names, and public member function names
  • SCREAMING_SNAKE_CASE for device types, preprocessor macros and constants
  • Device class names end in _device
  • Lowercase letters in hex constants/data

I’ll take a look at the current code later today and see if anything sticks out to me.

//------------------- END #defines ---------------------


unsigned long cs8900a_device::crc32_buf(const char *buffer, unsigned int len)
Copy link
Contributor

@pmackinlay pmackinlay Aug 25, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suggest you can use util::crc32_creator::simple() here instead of another implementation.

/** #define CS8900_DEBUG_RXTX_DATA 1 **/ /* enable to see data in/out flow */
/** #define CS8900_DEBUG_FRAMES 1 **/ /* enable to see arch frame send/recv */

#define log_message(level,...) fprintf(stderr,__VA_ARGS__)
Copy link
Contributor

@pmackinlay pmackinlay Aug 25, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggest #including "logmacro.h" and using the LOG_X and VERBOSE macros instead.

Copy link
Member

@cuavas cuavas Aug 25, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, see examples in other files that use logmacro.h – it’s better if this stuff is done in a consistent way. Also avoid using printf family functions in general, they behave differently on different targets and are not type-safe. It’s better to use logerror and MAME’s osd_printf_ family of functions,

Copy link
Member

@cuavas cuavas Aug 25, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

deb674f is a recent example of using logmacro.h


/* The packetpage register: see p. 39f */
#define CS8900_PP_ADDR_PRODUCTID 0x0000 /* R- - 4.3., p. 41 */
#define CS8900_PP_ADDR_IOBASE 0x0020 /* i RW - 4.3., p. 41 - 4.7., p. 72 */
Copy link
Contributor

@pmackinlay pmackinlay Aug 25, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider using enums rather than macros for these?

cs8900_set_receiver(0);
}

void cs8900a_device::device_start(void)
Copy link
Contributor

@pmackinlay pmackinlay Aug 25, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider saving state (almost making it visible to the debugger).

Copy link
Contributor Author

@roadriverrail roadriverrail Aug 25, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Gladly-- where can I find an example of doing this?

Copy link
Contributor Author

@roadriverrail roadriverrail Aug 25, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I think I found examples in other devices. Are there things that should and should not be saved?

Copy link
Contributor

@rb6502 rb6502 Aug 25, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The general rule for savestates is variables that are driven by runtime rather than by the normal setup of the device. So for instance you wouldn't save a ROM area because it's immutable and loaded at startup, but any CPU-writable registers and buffers you definitely would.

Copy link
Contributor Author

@roadriverrail roadriverrail Aug 25, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rb6502 Can do. For this, that's the IO registers, the PacketPage, and the class' internal state, so, basically, every member variable. The inbound frame queue is just a holding spot for frames delivered from the network subsystem so that subsequent reads can spool them off; that likely shouldn't be saved.

Copy link
Contributor

@rb6502 rb6502 Aug 25, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, because the external network won't come up in the same state.

}

/* ----- read byte from I/O range ----- */
u8 cs8900a_device::cs8900_read(u16 io_address)
Copy link
Contributor

@pmackinlay pmackinlay Aug 25, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggest guarding against side-effects for debugger reads by testing machine().side_effects_disabled() in here.

#ifdef CS8900_DEBUG
printf("SENDING from buf %p len %d\n", &cs8900_packetpage[CS8900_PP_ADDR_TX_FRAMELOC], tx_length);
#endif
send(&cs8900_packetpage[CS8900_PP_ADDR_TX_FRAMELOC],tx_length);
Copy link
Contributor

@pmackinlay pmackinlay Aug 25, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider using the send_complete_cb() to update the transmitter state when sending frames, to somewhat simulate the delay of getting those bits onto the wire.

Copy link
Contributor Author

@roadriverrail roadriverrail Aug 29, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As part of my latest changes, I did consider this. I have not done so at this point because: (1) this is how the VICE/TFE code did it, and it's the only "successful" emulation of the CS8900A I have to go on (2) I haven't been able to determine from the data sheet on the CS8900A if the VICE/TFE code is actually wrong. I'm happy to move the post-send side-effect code off into the send_complete_cb() callback and commit that if it "works for me", but I can't say that it's any more proper or not and I don't want to interfere too hard with a working implementation that I didn't have a close hand in writing myself.

@@ -2372,6 +2372,9 @@ if (BUSES["A2BUS"]~=null) then
MAME_DIR .. "src/devices/bus/a2bus/byte8251.h",
MAME_DIR .. "src/devices/bus/a2bus/cmsscsi.cpp",
MAME_DIR .. "src/devices/bus/a2bus/cmsscsi.h",
MAME_DIR .. "src/devices/bus/a2bus/uthernet.cpp",
MAME_DIR .. "src/devices/bus/a2bus/uthernet.h",

Copy link
Member

@cuavas cuavas Aug 25, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You’ve got a stray blank line here.

@@ -1019,6 +1019,19 @@ if (MACHINES["CS8221"]~=null) then
}
end

---------------------------------------------------
--
--@src/devices/machine/cs8221.h,MACHINES["CS8900A"] = true
Copy link
Member

@cuavas cuavas Aug 25, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The header path/name for triggering this with a SOURCES= build needs to be correct.

@@ -310,7 +310,6 @@ end
"bx",
"ocore_" .. _OPTIONS["osd"],
}

Copy link
Member

@cuavas cuavas Aug 25, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please back out this stray change.


#include "emu.h"
#include "emuopts.h"
#include "uthernet.h"
Copy link
Member

@cuavas cuavas Aug 25, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The module header should be the first thing included after the prefix header. It should start like this:

#include "emu.h"
#include "uthernet.h"

#include whatever other headers

If it doesn’t work like that, the header needs to be fixed.

// GLOBAL VARIABLES
//**************************************************************************

DEFINE_DEVICE_TYPE(A2BUS_UTHERNET, a2bus_uthernet_device, "uthernet", "Uthernet")
Copy link
Member

@cuavas cuavas Aug 25, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add the a2 prefix used by all the Apple II cards to the short name (third argument) – it should be "a2uthernet".

cs8900_set_tx_status(0,0);
}

void cs8900a_device::device_reset(void)
Copy link
Member

@cuavas cuavas Aug 25, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We generally don’t put void in the parentheses for functions that take no arguments, since C++ doesn’t support K&R-style functions.

#endif
/* always reads zero on HW */
return 0;
} else {
Copy link
Member

@cuavas cuavas Aug 25, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please be consistent with your style – most of your else are on the line following the } of the preceding compound statement, but this one is on the same line.

Copy link
Contributor Author

@roadriverrail roadriverrail Aug 25, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Roger that. I found a Google+Allman uncrustify and will be applying it after the functional fixes.

switch (ppaddress)
{
case CS8900_PP_ADDR_CC_RXCFG:
/* Skip_1 Flag: remove current (partial) tx frame and restore state */
if (content & 0x40) {
Copy link
Member

@cuavas cuavas Aug 25, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please be consistent with your style. You seem to mostly use no space between keywords and their controlling expressions (e.g. if(...) but here you have an intervening space (switch (... and if (...).

/* read/write from packet page register */

/* read a register from packet page */
u16 cs8900a_device::cs8900_read_register(u16 ppaddress)
Copy link
Member

@cuavas cuavas Aug 25, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This function is using a different indentation style to the rest of the file. Please use one tab per scope level.

}

/* --- read address filter register range --- */
else if(ppaddress<0x160) {
Copy link
Member

@cuavas cuavas Aug 25, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please don’t put extra lines between the closing } of a compound statement controlled by an if and an else – it can easily lead to confusion. If a developer has this at the bottom of their window/terminal they will see the blank line and assume that there’s no following else.

Should clear out all comments on the lua files from the most recent code
review.
roadriverrail added 13 commits Aug 26, 2020
Copyright holders adjusted to include all known authors past and
present.
Mame offers util::crc_creator::simple, so there is no need for the
custom implementation.
MAME has its own logging system, so all the log_message and printf calls have
been moved over to it, and likewise the conditional compile flags have been
removed since they're to be enabled via the MAME log system.
Various code cleanups to the uthernet-- changing its name to be on-style,
removing unused variables and includes, adding in the clock for the CS8900A
(even though we don't make use of it), etc.
Calling save_item( ) on the state variables of a class (except for those whose
transience is dependent on the environment, such as the inbound frame queue)
is good practice for maintaining visibility to the debugger.
Reads to this device can come from the debugger.  Under these circumstances,
side-effects of reads need to be suppressed so the debugger can read without
interfering with the internal state of the device.
The "uthernet_intef" option was a remnant from the original hack of the cs8900a
support; TFE has been stripped from its cs8900a emulation and no longer needs
the interface name to run libpcap calls, so the option has been removed.
Moved all the previous preprocessor defines for constants into some sort of
reasonable enum.
Because the VICE/TFE authors had a radically different style, and often had
multiple conflicting styles, the source was uncrustified to something close
to the Google C++ style with Allman braces.  Minor hand cleanup was done
afterwards (e.g. removal of whitespace lines between if and else blocks)
@roadriverrail
Copy link
Contributor Author

roadriverrail commented Aug 29, 2020

Okay...I've done my best to bring all the editorial feedback in. The changes were done as a set of small commits over the past several days just to keep all the work straight. I promise to squash it all down once all the feedback is resolved.

Copy link
Member

@cuavas cuavas left a comment

It’s looking better now. A couple of general things in addition to the comments:

  • Please re-indent the new files using tabs rather than spaces, with one tab per level of scope. (For C++, we use tabs for indenting, one tab per scope level, and display at 4 spaces per tab.)
  • There are still a few C-style foo c::bar(void) member functions – please change them to foo c::bar() for consistency with the rest of MAME.
  • There’s no need to squash the commits down yourself, it’s easy for us to do that when the PR is merged.

Thanks for addressing the feedback.

#include <cstring>

#include "machine/cs8900a.h"
Copy link
Member

@cuavas cuavas Aug 30, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please use conventional #include order – prefix header, module header, project headers, standard library headers:

#include "emu.h"
#include "machine/cs900a.h"

#include <cstring>

char * cs8900a_device::debug_outbuffer(const int length, const unsigned char *const buffer)
{
int i;
static const u32 MAXLEN_DEBUG = 1600;
static const u32 TFE_DEBUG_MAX_FRAME_DUMP = 150;
static char outbuffer[MAXLEN_DEBUG * 4 + 1];
char *p = outbuffer;

assert(TFE_DEBUG_MAX_FRAME_DUMP <= MAXLEN_DEBUG);

*p = 0;

for (i = 0; i < TFE_DEBUG_MAX_FRAME_DUMP; i++)
{
if (i >= length)
break;

sprintf(p, "%02X%c", buffer[i], ((i + 1) % 16 == 0) ? '*' : (((i + 1) % 8 == 0) ? '-' : ' '));
p += 3;
}

return outbuffer;
}
Copy link
Member

@cuavas cuavas Aug 30, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you make this use stream objects to avoid a static buffer and make it a bit safer? Something like this (untested, but it should give a starting point):

#include <sstream>

...

std::string cs8900a_device::debug_outbuffer(const int length, const unsigned char *const buffer)
{
  static constexpr u32 MAXLEN_DEBUG             = 1600;
  static constexpr u32 TFE_DEBUG_MAX_FRAME_DUMP = 150;
  std::ostringstream outbuffer;

  static_assert(TFE_DEBUG_MAX_FRAME_DUMP <= MAXLEN_DEBUG, "Max fame dump must not be larger than max debug length");

  for (int i = 0; (i < TFE_DEBUG_MAX_FRAME_DUMP) && (i < length); i++)
    util::stream_format(outbuffer, "%02X%c", buffer[i], ((i + 1) % 16 == 0) ? '*' : (((i + 1) % 8 == 0) ? '-' : ' '));

  return outbuffer.str();
}

Copy link
Contributor Author

@roadriverrail roadriverrail Aug 30, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking at this right now. How do I actually turn my logging on now that it's grafted into the MAME logging system? debug_outbuffer is called only by a couple of specific logging statements, and the MAME docs aren't super clear on how to enable particular modules' logging.

Copy link
Contributor

@rb6502 rb6502 Aug 30, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The -oslog switch at the command line turns on writing logs to the TTY, and I think there's some way to have it show in the debugger as well.

Copy link
Contributor Author

@roadriverrail roadriverrail Aug 30, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I actually set -log -oslog -verbose, and I did see logs for the CPU but not for my chipset, even with VERBOSE set to 1, which is what led me to ask in the first place. I'm fairly sure I followed the recommended pattern for logging in my code.

Copy link
Member

@cuavas cuavas Aug 31, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You need to set VERBOSE to the combination of log masks you want enabled, e.g.

#define VERBOSE (CS8900_DEBUG | CS8900_DEBUG_REGISTERS | CS8900_DEBUG_FRAMES)

If you set it to 1 it's equivalent to LOG_GENERAL and it will just enable stuff logged with the abbreviated LOG macro.

This is a helper for cs8900_receive() to determine if the received frame should be accepted
according to the settings.
*/
int cs8900a_device::cs8900_should_accept(unsigned char *buffer, int length, int *phashed, int *phash_index,
Copy link
Member

@cuavas cuavas Aug 30, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please use bool return type for true/false values.

The indentation style was cleaned up and some of the more aethetic code aspects
(e.g. lining equals signs up) was either tab-ified or simply removed.

Additionally, some K&R-style declarations were cleaned up and a line of dead code
was removed.
debug_outbuffer has been made a bit more C++-like and had its static buffer
removed.  Additionally, a number of int variables were being used as boolean
flags, so they've been changed to the bool type.
@cuavas
Copy link
Member

cuavas commented Aug 31, 2020

This is in a state where it can be merged for now. Any additional issues to follow up can still be posted here as comments.

@cuavas cuavas merged commit 49466dc into mamedev:master Aug 31, 2020
2 checks passed
@cuavas cuavas changed the title Add support for TFE Uthernet emulation (Linux only, currently) bus/a2bus: Added Uthernet card emulation for Apple IIgs Aug 31, 2020
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.

None yet

6 participants